Repository navigation
Reverting an actor keeps a snapshot atelet uploaded but had not yet reported - #3079
Zoe Zhao (zoez7) wants to merge 7 commits into
Conversation
| func (w *ActorWorkflow) recordTerminatedUpload(ctx context.Context, actorRef resources.ActorRef, actor *ateapipb.Actor, node string, state *ateletpb.ActorSnapshotState) (*ateapipb.Actor, error) { | ||
| applied := proto.CloneOf(actor.GetStatus()) | ||
| if err := applyAteletResponseState(ctx, applied, state); err != nil { | ||
| return nil, apierror.Internal("atelet on node %q reported a durable snapshot for Actor %s that conflicts with its record: %w", node, actorRef, err) | ||
| } | ||
| if proto.Equal(applied, actor.GetStatus()) { | ||
| return actor, nil | ||
| } |
There was a problem hiding this comment.
🤖 high 🔴 – recordTerminatedUpload can promote an unfinalized snapshot from a crashed SuspendActor
ensureMarkedSuspending appends a PENDING snapshot with Locality: "", and only ensureSuspendedFinalized sets Locality once the suspend commits SUSPENDED. If SuspendActor crashes after atelet.Checkpoint has already committed the snapshot on atelet (for example, when cleanUpAfterCheckpoint or ensureVolumesDetached fails, or when the worker pod dies before FinalizeSuspended commits as in TestSuspendActor_Finalized_KeepsCrashFromWorkerDelete), atelet can hold that unfinalized snapshot as LatestDurable (either immediately in UPLOAD_MODE_IMMEDIATE, or after a background upload in UPLOAD_MODE_DEFERRED).
When RevertActor later terminates the crashed actor, Terminate returns that unfinalized snapshot in state.LatestDurable. applyAteletResponseState marks it COMPLETED even though snap.GetLocality() == "", so ensureInProgressSnapshotDiscarded skips it and FinalizeReverted reverts the actor to the failed suspend's snapshot while releaseDroppedSnapshots deletes the previous completed snapshot.
A PENDING snapshot from a completed SuspendActor always has snap.GetLocality() != "" (compare snapshotToResume and ensureTagSourceSnapshotDurable). Should recordTerminatedUpload skip promoting a PENDING snapshot when snap.GetLocality() == "", leaving it PENDING for ensureInProgressSnapshotDiscarded to collect?
| // the day this was written* because today NodeVmsWithLocalSnapshots has at | ||
| // most one item. | ||
| if err := s.snapshots.Forget(actorUID); err != nil { | ||
| state, err := s.snapshots.Forget(actorUID) |
There was a problem hiding this comment.
🤖 should-fix 🟡 – Forget deletes the snapshot state file before PruneLocalCheckpoints and removeActorDirs run. If either fails, Terminate returns an error and drops the state. A retried Terminate then returns an empty state, so the revert deletes the upload as in progress. Keeping latest_durable on disk until the cleanup succeeds would let the retry return it.
There was a problem hiding this comment.
Fixed in 456f1cb. Forget now leaves only latest_durable in the state file, and RemoveActorDirs deletes it after the actor's other files. A Terminate retried after a failed cleanup returns it again. Nothing local and no report due stays on disk, so an atelet restarted mid-Terminate starts no background work on the actor.
| if errors.Is(err, store.ErrVersionConflict) { | ||
| return nil, apierror.Aborted("concurrent update conflict, please retry") |
There was a problem hiding this comment.
🤖 should-fix 🟡 – A version conflict here drops the state atelet just returned. A retried revert gets an empty state from Terminate, so it deletes the upload as in progress. Other writers can race a revert: the UpdateActor RPC doesn't take the lease, and crashBoundActor crashes a REVERTING actor whose worker pod goes away. Re-reading the actor and applying the state again on a conflict would keep it.
There was a problem hiding this comment.
crashBoundActor already takes the actor lease (workflow_worker_delete.go, workflow_reconcile_assignments.go), and #3131 makes UpdateActor take it too. With every writer under the lease, a conflict here means the revert lost its lease partway. A retry would not help then. leaseCtx is about to be canceled, and another operation now holds the actor. So this stays a single write that returns Aborted, like the other workflow writes.
|
nvm I saw #3094 is doing that for node gone situation. |
atelet can upload a snapshot before its report reaches ateapi, and ateapi refuses reports for a CRASHED actor. Revert then deleted the upload as in progress. Terminate now returns the actor's snapshot state as atelet forgets it, and revert records its latest durable snapshot COMPLETED before it discards in-progress snapshots.
…lize When atelet is unreachable but the latest local snapshot is already uploaded, revert recorded a detach before finalizing. FinalizeReverted already clears the node binding and every snapshot's locality, so the extra write only added a version conflict. Revert now goes straight on, and recordDetach loses its revert-only branch.
A suspend that crashes after atelet commits its checkpoint leaves a PENDING snapshot on no node. Terminate can report it durable, and revert then returned the actor to it and released the previous snapshot. recordTerminatedUpload now skips a PENDING snapshot on no node, so revert deletes it as in progress.
Forget deleted the snapshot state file before Terminate pruned the actor's checkpoints and directories. If either failed, a retried Terminate returned no latest durable snapshot, and revert deleted the upload as in progress. Forget now leaves only the latest durable snapshot in the state file, and RemoveActorDirs deletes it after the actor's other files. With nothing local and no report due, an atelet restarted mid-Terminate starts no background work on the actor.
0fbb3cb to
456f1cb
Compare
recordTerminatedUpload skipped only a PENDING snapshot on no node. It now records a snapshot only when the record places it on the actor's assigned node, which only a finalized suspend does. Any other reported snapshot stays PENDING, and the revert deletes it. It also returns early when Terminate reports no durable snapshot, and reads the node from the actor instead of taking it as an argument.
DeleteWorker terminated a released actor on its node but dropped the state Terminate returned. atelet forgets the actor as it terminates it, so a later revert got no upload back and deleted it as in progress. The crash now records that upload in the same write, with the same rules as a revert.
A report for a CRASHED actor failed with FailedPrecondition, which atelet does not retry. The upload then reached the record only through Terminate's response, and was lost if that response or the write after it failed. A report for a CRASHED actor now records its finished upload when the record places the snapshot on the reporting node. It does not apply a detach. Terminate's upload goes through the same check.
Zoe Zhao (zoez7)
changed the title
|
I'm reverting this one to draft, since I found another bug that needs to be addressed before this. |
atelet uploads a snapshot in the background and then reports it to ateapi. If the actor is reverted before ateapi records the report, the revert deletes the upload as in progress. ateapi refused reports for a CRASHED actor. Any
Terminatemakes atelet forget the actor, so its report is never sent.Terminatereturns the actor's latest durable snapshot. atelet keeps it in the state file until the rest of the cleanup finishes, so a retriedTerminatereturns it again.RevertActor, and the crash inDeleteWorkerwhen a worker pod is gone, record the uploadTerminatereturns.ReportActorSnapshotStatefor a CRASHED actor now records the finished upload instead of returning FAILED_PRECONDITION. It does not apply a detach.FinalizeRevertedalready clears the node binding.AI use: AI-assisted with Claude Code, which investigated the race and drafted the code, tests, and this description. I reviewed them.